Security fix: isolate health checks from admin API rate-limit bucket - #126
Security fix: isolate health checks from admin API rate-limit bucket#126cursor[bot] wants to merge 2 commits into
Conversation
Public GET /api/v1/health shared the authenticated /api/* rate-limit bucket, so unauthenticated health probes could exhaust the admin API budget for the same client IP and deny stream management requests. Co-authored-by: Alexander Wagner <info@alexanderwagnerdev.com>
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23023dca90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if path.starts_with("/api/") { | ||
| (self.config.api_max, "api") |
There was a problem hiding this comment.
When an attacker shares the resolved client IP with an administrator, they can still exhaust this bucket by repeatedly requesting a protected route such as GET /api/v1/streams without a bearer token. The rate-limit middleware runs before handle_streams_list performs authentication, so these 401 responses consume the same api budget and the next legitimate authenticated request receives 429. Thus, moving only /api/v1/health leaves the claimed admin API denial-of-service fix trivially bypassable; the protected budget needs to distinguish authenticated requests rather than only the health path.
Useful? React with 👍 / 👎.
| if path == "/api/v1/health" { | ||
| (self.config.default_max, "health") |
There was a problem hiding this comment.
When operators tune the published rate-limit settings, this silently makes /api/v1/health use HTTP_RATE_LIMIT_DEFAULT, although .env.example documents HTTP_RATE_LIMIT_API as covering /api/* and the default setting as covering all other routes. For example, a deployment with API=120 and DEFAULT=1 now returns 429 on its second health probe even though its documented /api/* allowance is 120. Either expose/document a health-specific setting or update the configuration contract so deployments do not unexpectedly break health monitoring.
Useful? React with 👍 / 👎.
| } else if path.starts_with("/stats") { | ||
| self.config.stats_max | ||
| (self.config.stats_max, "stats") |
There was a problem hiding this comment.
When the same client polls both /stats and /stats-nginx, this classification now gives both requests the identical {peer}:stats key. Before this commit the key was derived from the first path segment, so each endpoint independently allowed stats_max requests; a workload that legitimately made up to that many requests to each endpoint can now receive 429 after only stats_max combined requests. Preserve the previous per-endpoint keys for non-health routes unless this unrelated compatibility change is intentional.
Useful? React with 👍 / 👎.
2 participants
Superseded by #129, which includes this PR's fix unchanged plus fixes for the Codex review findings raised on it (unauthenticated requests to protected
/api/*routes no longer share the admin budget,.env.exampleupdated to match,/statsvs/stats-nginxbudgets kept independent).